Cmake improvements and Nix unification with existing pytket setup #1501

jake-arkinstall · 2024-07-25T10:55:50Z

Description

Presently, TKET's CMakeLists.txt declares its dependencies as private, even when they are depended upon in public headers. As such, CMake projects that import TKET as a dependency must also add those dependencies in order to #include <tket/Circuit/Circuit.hpp>, for example.

In this PR, such dependencies are now declared public in the target_link_libraries. This is also done for libs/tktokenswap and libs/tkwsm, such that projects depending on them should now need to add no additional dependencies.

With these changes in place, the CMakeLists.txt for tests and for pytket could be simplified, and this is done in this PR too. This provided an opportunity to simplify the Nix build process, and this has been done. Nix now builds pytket through the CMake path in setup.py instead of the custom NixBuild path. This does require some minor changes to the CMakeBuild, but I believe they are inconsequential to non-nix builds (CI will verify):

A user can now pass a BUILD_DIR environment variable to setup.py to specify where to build the cmake project in setup.py. This is because the rmtree command was also removing python setup outputs within a nix build, as those happened to also be placed inside build/. By providing e.g. a temporary directory, Nix can avoid this problem while non-nix users still have their builds default to the build directory.
Instead of removing the build directory and then making it, it is now simply stripped of existing files. This helps in the situation when a BUILD_DIR is provided in the environment with insufficient removal permissions, and also avoids a potential problem wherein a terminal session open in the build directory becomes homeless, rather than simply observing files being removed.

tests/CMakeLists.txt is also simplified by globbing test files, rather than listing them manually. This is an opinionated change.

There was an inconsistency in #include styles within headers. Files were sometimes included by name only, sometimes by a relative path, and sometimes as relative to the respective project root. I have modified these to follow the 'respective project root' approach, as this is the most explicit, and most tooling-friendly. However, this introduces a lot of small changes in a lot of files, and may introduce merge conflicts if incoming PRs might make header adjustments in affected regions. As such, it might be preferable to eliminate those changes.

Related issues

#1482

Checklist

I have performed a self-review of my code.
I have commented hard-to-understand parts of my code.
I have made corresponding changes to the public API documentation.
I have added tests that prove my fix is effective or that my feature works.
I have updated the changelog with any user-facing changes.

doug-q · 2024-07-26T08:16:27Z

pytket/setup.py

        ext_suffix = get_config_var("EXT_SUFFIX")
-        lib_names.extend(f"{binder}{ext_suffix}" for binder in binders)
+        lib_names = [f"{binder}{ext_suffix}" for binder in binders]


You have removed libtket.so from lib_names here, is that intentional?

It is - the binders are already linked to tket.

So if I run nix build .#pytket and it outputs, say, /nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0, I get:

find /nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0/ -name "*.so" | xargs ldd /nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0/lib/python3.12/site-packages/pytket/_tket/architecture.cpython-312-x86_64-linux-gnu.so: linux-vdso.so.1 (0x00007ffebeffa000) libtket.so => /nix/store/r83mq87lf2jyzdzhb25v9kslqf1ccdgk-tket/lib/libtket.so (0x00007fef3c200000) libsymengine.so.0.11 => /nix/store/fww6693dlgs4fxcfcaib2md8w7sfp6r2-symengine-0.12.0/lib/libsymengine.so.0.11 (0x00007fef3ba00000) libflint.so.17 => /nix/store/bpryc1gw9a15nmdk3036jmdgqh4y0ix0-flint-2.9.0/lib/libflint.so.17 (0x00007fef3ae00000) libmpc.so.3 => /nix/store/s2cb7sfy1l5d7j1c0dryk4j4spq5adp4-libmpc-1.3.1/lib/libmpc.so.3 (0x00007fef3cc12000) libmpfr.so.6 => /nix/store/2wrknbiwjkrhywgfzc2wjd273bghv0xb-mpfr-4.2.1/lib/libmpfr.so.6 (0x00007fef3cb5a000) libgmp.so.10 => /nix/store/ip431mx66mcnbbzfphfzvr1h61vddc2q-gmp-with-cxx-6.3.0/lib/libgmp.so.10 (0x00007fef3cab5000) libtktokenswap.so => /nix/store/ylz20wz0q4pi8k9fcvaap867ddyhm1r2-tktokenswap/lib/libtktokenswap.so (0x00007fef3c185000) libtkassert.so => /nix/store/hjm2nfl2k8s6i78a7a080r42pld268f0-tkassert/lib/libtkassert.so (0x00007fef3caaf000) libtklog.so => /nix/store/vqgqfqwkhbbvgibiifhfi0ppwayf6wrw-tklog/lib/libtklog.so (0x00007fef3caa6000) libtkrng.so => /nix/store/l2k1id5xa9x6ps6difknkkfyx2fdgd4q-tkrng/lib/libtkrng.so (0x00007fef3caa0000) libstdc++.so.6 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libstdc++.so.6 (0x00007fef3aa00000) libm.so.6 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libm.so.6 (0x00007fef3c09f000) libgcc_s.so.1 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libgcc_s.so.1 (0x00007fef3c07a000) libc.so.6 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6 (0x00007fef3a809000) libtkwsm.so => /nix/store/8s49jk9li42z583k2qgiyrpzvkdd7bh5-tkwsm/lib/libtkwsm.so (0x00007fef3b978000) libopenblas.so.0 => /nix/store/iy0g9kndccxpknqk8flsvzhqzjni26rd-openblas-0.3.27/lib/libopenblas.so.0 (0x00007fef38da8000) libntl.so.44 => /nix/store/sm8zhkpz3rrdldfknm4w5zz994lvsrfg-ntl-11.5.1/lib/libntl.so.44 (0x00007fef38a00000) libpthread.so.0 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libpthread.so.0 (0x00007fef3ca97000) /nix/store/dbcw19dshdwnxdv5q2g6wldj6syyvq7l-glibc-2.39-52/lib64/ld-linux-x86-64.so.2 (0x00007fef3cca4000) libgfortran.so.5 => /nix/store/wmina0mbakp0vkv4amkha9sv8zj9bkx2-gfortran-13.3.0-lib/lib/libgfortran.so.5 (0x00007fef38600000) libgomp.so.1 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libgomp.so.1 (0x00007fef3c02d000) libgf2x.so.3 => /nix/store/bx8mvbs9w946xs5csfh5ijl43ggd98p8-gf2x-1.3.0/lib/libgf2x.so.3 (0x00007fef3b95c000) libdl.so.2 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libdl.so.2 (0x00007fef3ca90000)

I believe this to be sufficient, as the cmake builds of pytket are working on nix, ubuntu and macos.

The windows conan build, however, is failing, and and I'm scratching my head over that as the changes to the conan build are trivial.

cqc-alec · 2024-08-13T10:49:40Z

Thanks Jake, looks good. When testing locally I encountered a couple of issues (following my usual local workflow).

First, when building tket with BUILD_TKET_TEST set:

cd tket
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=/home/alec/local -DBUILD_TKET_TEST=1 ..
cmake --build . -j10

I get linker errors, seemingly related to gmp (which I have installed).

Secondly. when trying to build pytket and run the tests (after first building shared-library versions of tklog and tket):

cd pytket
NO_CONAN=1 INSTALL_DIR=/home/alec/local PYTKET_CMAKE_N_THREADS=10 pip install -v -e .[zx]
cd tests
pytest

I get a lot of errors like this:

____________________________ ERROR collecting tests/add_circuit_test.py _____________________________
ImportError while importing test module '/home/alec/r/tket/pytket/tests/add_circuit_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
add_circuit_test.py:15: in <module>
    from pytket import Circuit, OpType
../pytket/__init__.py:17: in <module>
    from pytket.circuit import (
../pytket/circuit/__init__.py:29: in <module>
    from pytket._tket.circuit import *
E   ImportError: /home/alec/r/tket/pytket/pytket/_tket/libtket.so: undefined symbol: __gmpn_perfect_square_p

Again this seems gmp-related.

cqc-alec

No issue with the changes; a merge conflict needs resolving; and I'd like to understand why my usual local methods of building and testing aren't working (as noted in the comment).

Previously Nix had its own special build, but with the changes to tket's CMake this is no longer necessary. As such, Nix now uses the CMake build path, which has only had to change in one manner: ./build had to be renamed to ./cmake_build, so as not to remove python files previously placed there by setup.py.

Previously, the build directory being CWD/build caused problems in the pytket setup.py process in a Nix build. Changing this to CWD/cmake_build fixed the issue temporarily. Now I am defaulting back to CWD/build, but allowing another directory to be provided with a BUILD_DIR environment variable. This means the setup should be, for all intents and purposes, identical on non-nix builds.

Previously tests were compiled into executables, but not registered with ctest using catch's catch_discover_tests. I've added that in, and added enable_testing to tket's CMakeLists.txt in the presence of test flags. This also allows for a simplification of the nix derivation - TKET and its tests are now built as one project, then checked.

…ng tests in the test directory to allow for relative paths

cqc-alec · 2024-08-13T14:33:20Z

Probably need to add it to pytket's CMakeLists.txt as well?

jake-arkinstall · 2024-08-13T14:36:22Z

I get linker errors, seemingly related to gmp (which I have installed).
...
Again this seems gmp-related.

Interesting. I think this goes back to SymEngine choosing not to expose its own dependencies, so it has to be added in. It doesn't appear that this is always necessary - CI didn't have an issue, and neither did the nix build. I've done so on Tket directly so pytket doesn't need to add it as was the case previously.

I've rebased from origin/main to resolve the conflict. I'd appreciate it if you could give it another go.

cqc-alec · 2024-08-13T14:47:30Z

I'd appreciate it if you could give it another go.

Adding gmp as a direct dependency of tket has fixed the first issue, but I still get the same runtime issue with pytket.

jake-arkinstall · 2024-08-13T15:01:54Z

I'll spawn a docker build and see what's going on!

github-actions · 2024-09-17T05:22:36Z

This pull request has been automatically marked as stale.

jake-arkinstall requested a review from cqc-alec July 25, 2024 10:55

jake-arkinstall linked an issue Jul 25, 2024 that may be closed by this pull request

Investigate potential CMake improvements #1482

Closed

doug-q reviewed Jul 26, 2024

View reviewed changes

jake-arkinstall force-pushed the 1482-investigate-potential-cmake-improvements branch from 4913ad0 to 9abb317 Compare July 26, 2024 15:57

cqc-alec reviewed Aug 13, 2024

View reviewed changes

jake-arkinstall added 16 commits August 13, 2024 15:21

Unify include paths in C++ 'libs' headers

6243cd4

Set libraries required by TKET dependencies as PUBLIC

1bccba2

Reformatted setup.py

c481e58

Revert prior export removal in CMakeLists.txt

6a5eac2

Remove now-defunct includes-fixup (include paths are now uniform)

85056ad

Reverted previous disabling of pytket testing upon build

f95582b

Applying clang-format

13802cc

Updated conanfile to allow cmake to handle the test json files, runni…

928a6c0

…ng tests in the test directory to allow for relative paths

Fixed nix pytket testing environment

a96e487

Fix windows tket build and pytket import issue.

ea4bac7

Adding output to valgrind check to diagnose missing file issue

a84a14e

Correcting the test data directory handling and installation

7513cbf

re-added gmp as a direct dependency to TKET

8598141

jake-arkinstall force-pushed the 1482-investigate-potential-cmake-improvements branch from 06b6e51 to 8598141 Compare August 13, 2024 14:35

Formalise test building variables as cmake options

9c98f60

github-actions bot added the stale label Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake improvements and Nix unification with existing pytket setup #1501

Cmake improvements and Nix unification with existing pytket setup #1501

jake-arkinstall commented Jul 25, 2024

doug-q Jul 26, 2024

jake-arkinstall Jul 26, 2024

cqc-alec commented Aug 13, 2024

cqc-alec left a comment

cqc-alec commented Aug 13, 2024

jake-arkinstall commented Aug 13, 2024 •

edited

Loading

cqc-alec commented Aug 13, 2024

jake-arkinstall commented Aug 13, 2024

github-actions bot commented Sep 17, 2024

Cmake improvements and Nix unification with existing pytket setup #1501

Are you sure you want to change the base?

Cmake improvements and Nix unification with existing pytket setup #1501

Conversation

jake-arkinstall commented Jul 25, 2024

Description

Related issues

Checklist

doug-q Jul 26, 2024

Choose a reason for hiding this comment

jake-arkinstall Jul 26, 2024

Choose a reason for hiding this comment

cqc-alec commented Aug 13, 2024

cqc-alec left a comment

Choose a reason for hiding this comment

cqc-alec commented Aug 13, 2024

jake-arkinstall commented Aug 13, 2024 • edited Loading

cqc-alec commented Aug 13, 2024

jake-arkinstall commented Aug 13, 2024

github-actions bot commented Sep 17, 2024

jake-arkinstall commented Aug 13, 2024 •

edited

Loading